Propagate Function Invocations#216
Conversation
rcosta358
added
enhancement
rcosta358
changed the title
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Is using strings here the best idea? Couldn't we use the expressions?
Also, we need more examples.
I was wondering, for the test suite we have already, we dont check the simplification right? we only have tests for "if an error in that line of type T", should we plan for a startegy to also have more tests for simplification?
| Expression left = be.getFirstOperand(); | ||
| Expression right = be.getSecondOperand(); | ||
| String leftKey = substitutionKey(left); | ||
| String rightKey = substitutionKey(right); |
There was a problem hiding this comment.
Is using strings for substitution really the best approach?
| ValDerivationNode result = ExpressionSimplifier.simplify(fullExpression); | ||
|
|
||
| assertEquals("size(x3) == 0", result.getValue().toString()); | ||
| } |
There was a problem hiding this comment.
we need a couple more tests for this, I'm not very convinced
|
The mapping we had was |
Can we improve those tests to show the string? @Test
void testFunctionInvocationEqualitiesPropagateTransitively() {
String toTest = "size(x3) == size(x2) - 1 && size(x2) == size(x1) + 1 && size(x1) == 0";
Expression expr = RefinementsParser.createAST(toTest, "");
ValDerivationNode result = ExpressionSimplifier.simplify(expr);
assertEquals("size(x3) == 0", result.getValue().toString());
}its pretty hard to understand the tests the way they are now |
|
Will do. |
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
I would still prefer the map Expr->Expr than String->Expr I think it makes more sense, and we can use equals so I dont think it adds complexity and its more accurate.
But this should work for now if you want to take care of that in another PR, or ask for someone else's opinion
| } else if (left instanceof Var var && canSubstitute(var, right)) { | ||
| map.put(var.getName(), right.clone()); | ||
| } else if (left instanceof FunctionInvocation && !right.toString().contains(leftKey)) { | ||
| map.put(leftKey, right.clone()); |
There was a problem hiding this comment.
this means we would not substitute something like:
f(a) == ff(a) + b
because the string on the right contains f(a) even though they are not related, right?
Ig better be conservative but we should be aware of this limitation
There was a problem hiding this comment.
You're right, I'll replace that string check with an AST check.
2 participants
Description
This PR allows function invocations to be on the LHS of mappings (instead of just variables).
This works by using the function string representation as a key in the variable resolver map, meaning they behave as if they were variables.
For example,
func(a) == func(b) && func(b) == 1→func(a) == 1.Example
Before
After
Related Issue
None.
Type of change
Checklist
ExpressionSimplifierTestmvn testpasses locally